-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support of keyspace
field for BATCH message
#1778
base: trunk
Are you sure you want to change the base?
Conversation
82b05d3
to
fcd5565
Compare
fcd5565
to
6224a0b
Compare
keyspace
field for BATCH message
conn.go
Outdated
@@ -1554,6 +1554,10 @@ func (c *Conn) executeBatch(ctx context.Context, batch *Batch) *Iter { | |||
customPayload: batch.CustomPayload, | |||
} | |||
|
|||
if c.version > protoVersion4 { | |||
req.keyspace = c.currentKeyspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of introducing keyspace
field in the protocol is not to copy default keyspace that we used when establishing connection, but to be able to override keyspace on per-statement basis. You need to specify keyspace from Batch
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with you. I added the SetKeyspace
field for the Batch
to allow overriding keyspace for the specific batch.
cassandra_test.go
Outdated
t.Skip("keyspace for BATCH message is not supported in protocol < 5") | ||
} | ||
|
||
err := createTable(session, "CREATE TABLE batch_keyspace(id int, value text, PRIMARY KEY (id))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should create table in different keyspace, and then do b.keyspace = "foo"
. Query session.Query("SELECT * FROM batch_keyspace")
should also override the default keyspace.
cassandra_test.go
Outdated
defer iter.Close() | ||
|
||
for i := 0; iter.Scan(&id, &text); i++ { | ||
if id != ids[i] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use require.Equal(t, ...)
.
texts := []string{"val1", "val2"} | ||
|
||
b := session.NewBatch(LoggedBatch) | ||
b.Query("INSERT INTO batch_keyspace(id, value) VALUES (?, ?)", ids[0], texts[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also do the test when one of queries also overrides the keyspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see from the proto 5 spec we can not override the keyspace for the specific query in the Batch, only for the Batch itself.
4.1.7. BATCH
Allows executing a list of queries (prepared or not) as a batch (note that
only DML statements are accepted in a batch). The body of the message must
be:
<type><n><query_1>...<query_n><consistency><flags>[<serial_consistency>][<timestamp>][<keyspace>][<now_in_seconds>]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that my comment might not be understandable. You can simulate hardcoding keyspace in CQL string itself. But, yeah it is not a very useful test indeed.
… that the query should be executed on
@lukasz-antoniak Thanks a lot for your feedback! I added some enhancements according to your review. Could you please take a look at this? |
Can this be closed in favor of #1822 ? |
Hello👋. I don't see any problem of closing this one and others PRs as well 👍 |
This PR adds support for the keyspace field for the BATCH message as part of Native Protocol v5 Support.
This implementation is based on the current implementation for QUERY and EXECUTE messages in gocql.